Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(hesai): add high resolution mode setting for Pandar128 and OT128 #142

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

feat(hesai): add high resolution mode setting for Pandar128 and OT128 #142

wants to merge 13 commits into from

Conversation

mojomex
Copy link
Collaborator

@mojomex mojomex commented Apr 24, 2024

PR Type

  • New Feature

Related Links

TIER IV INTERNAL LINK -- Jira ticket

This PR depends on the following PR being merged first:

Description

This PR adds a launch parameter hires_mode (boolean) which is used to enable/disable high resolution mode for Pandar128 and OT128.
The hardware interface has been extended to send the corresponding PTC commands.

Standard Mode High Resolution Mode
image image

Review Procedure

Test with Pandar128 and OT128 and confirm that horizontal resolution changes like in the images above:

  • launching with hires_mode:=false
  • launching with hires_mode:=true
  • running ros2 param set /hesai_hw_driver hires_mode true during runtime
  • running ros2 param set /hesai_hw_driver hires_mode false during runtime

Remarks

🟡 I currently do not have a Pandar128 to test with, and also no access to its TCP documentation. It still needs to be confirmed if the command is the same as for OT128.

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

  • Assign PR to reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

  • Commits are properly organized and messages are according to the guideline
  • (Optional) Unit tests have been written for new behavior
  • PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

  • All open points are addressed and tracked via issues or tickets

CI Checks

  • Build and test for PR: Required to pass before the merge.

@mojomex mojomex requested review from amc-nu and drwnz April 24, 2024 09:42
@mojomex
Copy link
Collaborator Author

mojomex commented Apr 25, 2024 •
edited
Loading

🟢 Evaluation

🟢 OT128

Tested:

  • starting Nebula with hires_mode:=false, then setting it to true, false, true via ros2 param set /hesai_hw_driver hires_mode true/false

  • starting Nebula with hires_mode:=true, then setting it to false, true, falseviaros2 param set /hesai_hw_driver hires_mode true/false`

  • Confirmed TCP communication is correct using Wireshark and other tools.

  • Confirmed visually that hires mode is indeed changed

🟢 Pandar128

Not tested yet, but after consulting with @drwnz, merging is still fine.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 240 lines in your changes are missing coverage. Please review.

Project coverage is 10.95%. Comparing base (eb5e1b7) to head (55ce98a).
Report is 4 commits behind head on main.

Files Patch % Lines
.../nebula_hesai_hw_interfaces/hesai_hw_interface.cpp 0.00% 165 Missing ⚠️
.../nebula_hw_interfaces_hesai/hesai_cmd_response.hpp 0.00% 52 Missing ⚠️
...a_ros/src/hesai/hesai_hw_interface_ros_wrapper.cpp 0.00% 12 Missing ⚠️
...ula_ros/src/hesai/hesai_hw_monitor_ros_wrapper.cpp 0.00% 10 Missing ⚠️
.../nebula_hw_interfaces_hesai/hesai_hw_interface.hpp 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff            @@
##            main     #142      +/-   ##
=========================================
+ Coverage   4.84%   10.95%   +6.10%     
=========================================
  Files        249       81     -168     
  Lines      19210     9424    -9786     
  Branches    1075     1203     +128     
=========================================
+ Hits         931     1032     +101     
+ Misses     17579     7536   -10043     
- Partials     700      856     +156
Flag Coverage Δ
differential 10.95% <0.00%> (?)
total ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mojomex mojomex self-assigned this Jun 13, 2024
mojomex added 10 commits July 4, 2024 18:54
@mojomex
fix(hesai_hw_interface): add error handling
fbd97f9 
* check PTC command error codes, throw exception if necessary
* perform size checks before parsing responses
* emit errors on too-large payload size
@mojomex
fix(hesai_hw_monitor_ros_wrapper): fixed wrong range given for S/N copy
c7cb1cb
@mojomex
fix(hesai): print uint8, uint16 as numbers
3b53806
@mojomex
chore: update cspell ignore
8b00b47
@mojomex
chore(velodyne_calibration_decoder): fix spelling
87b75e5
@mojomex
chore(velodyne_calibration_decoder): fix spelling once and for all
a3e2355
@mojomex
fix(hesai_hw_interface): add missing check for PTC error, make error …
378f4b2 
…type more readable
@mojomex
feat(hesai): add support for setting hires mode for Pandar128 and OT128
b5f0613 
Signed-off-by: Max SCHMELLER <[email protected]>
@mojomex
fix(hesai): add hires_mode to parameter schema
4c07b3d
@mojomex
chore: remove erroneously committed dependency directories
d996e03
@mojomex mojomex changed the base branch from main to develop July 4, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amc-nu amc-nu Awaiting requested review from amc-nu

@drwnz drwnz Awaiting requested review from drwnz

Assignees

@mojomex mojomex

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mojomex @codecov-commenter